Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

CodeMirror update #12940

Closed
wants to merge 3 commits into from
Closed

CodeMirror update #12940

wants to merge 3 commits into from

Conversation

zaggino
Copy link
Contributor

@zaggino zaggino commented Nov 23, 2016

PR for #12937

@zaggino zaggino mentioned this pull request Nov 23, 2016
@ficristo
Copy link
Collaborator

ficristo commented Dec 1, 2016

Why not use requirejs to create some logical mapping between the node_modules and thirdparty folders?
I think @petetnt has done that in #12006
If we go on this route we probably need to do something for distribution. (I don't remember exactly but I think we don't copy the node_modules over there)

@zaggino
Copy link
Contributor Author

zaggino commented Dec 1, 2016

Not sure how to do any logical mapping here, but happy to learn. We could use a separate package.json file inside the src folder for production dependencies. That's how brackets-electron does it and that's how electron authors recommend electron projects to be done.

@ficristo
Copy link
Collaborator

ficristo commented Dec 1, 2016

The other idea I had in mind was exactly putting a package.json inside the src folder.
Both cases seems better to me.
For the logical mapping @petetnt knows more.

@petetnt
Copy link
Collaborator

petetnt commented Dec 1, 2016

One can use relative mapping with ../node_modules/... for the requirejs config so you can use deps from the root folder: https://github.com/petetnt/brackets/blob/petetnt/poc-thirdparty-with-npm/src/main.js#L68. Not sure if that works in browser though.

@@ -323,7 +323,7 @@ module.exports = function (grunt) {
});

// task: install
grunt.registerTask('install', ['write-config', 'less', 'npm-install-extensions']);
grunt.registerTask('install', ['write-config', 'less', 'copy-browser-dependencies', 'npm-install-extensions']);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fast note: the script is referenced here as copy-browser-dependencies but is actually named move-browser-dependencies.

@zaggino
Copy link
Contributor Author

zaggino commented Dec 12, 2016

Closing in favor of #12972

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants